- 
                Notifications
    You must be signed in to change notification settings 
- Fork 0
Abstract record builder #90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR abstracts the record builder implementation by introducing a generic BuilderItem typeclass and a new MemPackHeaderOffset typeclass. The changes enable the builder state machine to work with different item types while maintaining a consistent interface.
Key changes:
- Introduced MemPackHeaderOffsettypeclass to make header offset configurable per type
- Created generic BuilderItemtypeclass with associatedParameterstype family
- Refactored ChunksBuilder.InMemoryto use the new genericBuilder.InMemoryimplementation
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description | 
|---|---|
| scls-format/src/Cardano/SCLS/Internal/Serializer/MemPack.hs | Added MemPackHeaderOffsettypeclass and implemented it forRawBytes | 
| scls-format/src/Cardano/SCLS/Internal/Serializer/Builder/InMemory.hs | New generic builder implementation with BuilderItemtypeclass | 
| scls-format/src/Cardano/SCLS/Internal/Serializer/ChunksBuilder/InMemory.hs | Refactored to use generic builder, implementing BuilderItemforChunkItem | 
| scls-format/src/Cardano/SCLS/Internal/Entry.hs | Implemented MemPackHeaderOffsetforChunkEntry | 
| scls-format/src/Cardano/SCLS/Internal/Serializer/Reference/Impl.hs | Added MemPackHeaderOffsetconstraint to serialize function | 
| scls-format/src/Cardano/SCLS/Internal/Serializer/Reference/Dump.hs | Added MemPackHeaderOffsetconstraint to affected functions | 
| scls-format/src/Cardano/SCLS/Internal/Serializer/External/Impl.hs | Added MemPackHeaderOffsetconstraint and updated imports | 
| scls-format/test/ChunksBuilderSpec.hs | Refactored tests to use helper function mkMachine' | 
| scls-format/scls-format.cabal | Added new module to exposed modules list | 
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
        
          
                scls-format/src/Cardano/SCLS/Internal/Serializer/ChunksBuilder/InMemory.hs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | In general I like the PR. But to be honest it took some time for me to understand the abstraction behind and how we are going to use it. So let me phrase it here, to be sure we are on the same page: 
 We are going to use it in order to be able to keep different chunk encoding for example raw or zstd. If so I'm ok, however I see that it seems it misses one scenario: 
 | 
| 
 The idea is to be quite more general. Specifically, I would like to use this logic to also build metadata records, from metadata entries. I wanted to avoid duplicating much of the code for this purpose, and that's the motive for this PR. In other words, I would phrase it like: 
 Regarding 
 Hmm true, in this current implementation we lose access to custom entry encoding. We could require an entry encoding function on the  Regardless, I'm open to discuss this further because I feel this might be a case of "Don't Repeat Yourself" vs. "So much complexity in software comes from trying to make one thing do two things." | 
| I totally agree with the current approach and extending is as complex as adding one extra method to the type class, so I'm fine with the approach | 
1dc4676    to
    7f3a29b      
    Compare
  
    
No description provided.